Skip to content

HBASE-30180: Can still add data to read-only region after flipping read-only flag multiple times#8280

Open
kgeisz wants to merge 1 commit into
apache:masterfrom
kgeisz:HBASE-30180-can-add-data-to-read-only-table
Open

HBASE-30180: Can still add data to read-only region after flipping read-only flag multiple times#8280
kgeisz wants to merge 1 commit into
apache:masterfrom
kgeisz:HBASE-30180-can-add-data-to-read-only-table

Conversation

@kgeisz
Copy link
Copy Markdown
Contributor

@kgeisz kgeisz commented May 28, 2026 •
edited
Loading

https://issues.apache.org/jira/browse/HBASE-30180

Note: Unit tests were generated using AI

  • testRegionOnConfigurationChangeLoadsReadOnlyCoprocessorsAfterRepeatedToggle() was generated with Cursor
  • testRegionCoprocessorHostUsesCompoundConfigurationAfterConfigChange() was generated with Claude Opus

Summary

This pull request fixes an issue with the new Read-Replica feature where if you create a table on the active cluster and flip the read-only flag multiple times, then sometimes it is still possible to add data to a table despite the cluster being in read-only mode. This issue was happening due to how the configuration for HRegion was being updated after running update_all_config in the HBase shell.

Key Changes

  • Delete the CoprocessorConfigurationUtil.updateCoprocessorListInConf() method since running this in HMaster, HRegionServer, and HRegion's onConfigurationChange() method is redundant.
    • This method was also improperly unsetting the region coprocessors for HRegion's CompoundConfiguration, which was the main cause of HBASE-30180.
  • Properly create a new RegionCoprocessorHost object in HRegion.onConfigurationChange() after a dynamic configuration update.
    • The new object was being created with the dynamically updated Configuration object provided as an arg in HRegion.onConfigurationChange(). However, it needs to be created with an updated version of HRegion's this.conf CompoundConfiguration object.
  • Add documentation to HMaster, HRegionServer, and HRegion's onConfigurationChange() methods regarding how the Configuration objects work.
    • For HMaster and HRegionServer, the Configuration object provided in their onConfigurationChange() methods is the same object reference as their respective this.conf Configuration objects.
  • Rename some variables for more clarity.

More Details

Read-only mode is determined by checking whether the ReadOnly coprocessors are loaded on the cluster. If they are present, then read-only mode is active, and vice-vera if they are not present. HBASE-30180 was being caused by how the CoprocessorConfigurationUtil.updateCoprocessorListInConf() method was unsetting the read-only coprocessors. HRegion's this.conf object is of type CompoundConfiguration. This differs from HMaster and HRegionServer, which use type Configuration. When disabling read-only mode, the updateCoprocessorListInConf() was running unset() on the configuration object, which does not properly unset the config property for CompoundConfigurations. Instead, the property needs to explicitly be set to an empty string.

After further investigation, the updateCoprocessorListInConf() method was redundant and not necessary since the read-only coprocessors were already being added/removed in CoprocessorConfigurationUtil.syncReadOnlyConfigurations(). That is why this PR removes the updateCoprocessorListInConf() entirely.

One additional issue was with how HRegion's this.coprocessorHost object was being updated after a dynamic configuration update. The new RegionCoprocessorHost object created for this.coprocessorHost was using the dynamically updated Configuration object when it should have been using an updated version of HRegion's CompoundConfiguration object. This PR corrects that discrepancy.

@kgeisz kgeisz force-pushed the HBASE-30180-can-add-data-to-read-only-table branch from 3f2cc32 to 024d94c Compare May 28, 2026 00:53
@taklwu taklwu requested review from Copilot and taklwu May 28, 2026 01:20
Copilot AI reviewed May 28, 2026
View reviewed changes
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes HBASE-30180 where toggling the read-only flag multiple times could leave a region writable. The root cause is that HRegion.this.conf is a CompoundConfiguration distinct from the newConf passed to onConfigurationChange(). The previous code synced the read-only coprocessor list onto newConf and built a new RegionCoprocessorHost from newConf rather than from this.conf, missing the table-descriptor layer and leaving stale data. For HMaster/HRegionServer, this.conf and newConf are the same object, so the issue is region-specific.

Changes:

  • Overload maybeUpdateCoprocessors to accept a separate confToUpdate, used for syncing read-only coprocessors and passed into the reload lambda.
  • Pass this.conf (the CompoundConfiguration) as confToUpdate in HRegion.onConfigurationChange so the new RegionCoprocessorHost is built from the correct config layer; remove the now-unneeded updateCoprocessorListInConf helper.
  • Simplify the HMaster/HRegionServer call sites since their this.conf == newConf.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
hbase-server/src/main/java/org/apache/hadoop/hbase/util/CoprocessorConfigurationUtil.java Adds confToUpdate parameter to maybeUpdateCoprocessors, removes obsolete updateCoprocessorListInConf helper, gates syncReadOnlyConfigurations on read-only-mode change only.
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java Passes this.conf (CompoundConfiguration) as confToUpdate and uses it for rebuilding RegionCoprocessorHost.
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java Simplifies lambda since this.conf == newConf.
hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java Same simplification as HRegionServer.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

taklwu
taklwu requested changes May 28, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@taklwu taklwu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Would you be able to come up with a unit test to prevent this error from happening?

Comment thread hbase-server/src/main/java/org/apache/hadoop/hbase/util/CoprocessorConfigurationUtil.java Outdated
CoprocessorConfigurationUtil.syncReadOnlyConfigurations(newConf, coprocessorConfKey);
reloadTask.reload(newConf);
componentName, instance);
if (hasReadOnlyModeChanged) {
Copy link
Copy Markdown
Contributor

@taklwu taklwu May 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this reminded me about #7514 were facing the same issue, IMO you're doing the right change.

@kgeisz kgeisz force-pushed the HBASE-30180-can-add-data-to-read-only-table branch from 7d0ffb5 to ffde80b Compare June 3, 2026 02:02
@kgeisz kgeisz requested a review from taklwu June 3, 2026 05:22
@kgeisz
Copy link
Copy Markdown
Contributor Author

kgeisz commented Jun 3, 2026

The unit test failures are not relevant.

@kgeisz kgeisz force-pushed the HBASE-30180-can-add-data-to-read-only-table branch from ffde80b to e8d50c6 Compare June 3, 2026 18:01
@kgeisz
Copy link
Copy Markdown
Contributor Author

kgeisz commented Jun 3, 2026 •
edited
Loading

I force-pushed another commit to fix a poorly formatted comment. We can run the tests again if needed. However, all relevant tests passed on the last run.

I also updated the PR description.

@kgeisz kgeisz force-pushed the HBASE-30180-can-add-data-to-read-only-table branch from e8d50c6 to 82a3576 Compare June 4, 2026 02:01
@kgeisz
Copy link
Copy Markdown
Contributor Author

kgeisz commented Jun 4, 2026 •
edited
Loading

Two more additions:

  • Gave HRegion.getConfiguration() the @RestrictedApi annotation
  • Added unit test: testRegionCoprocessorHostUsesCompoundConfigurationAfterConfigChange()
    • This test ensures HRegion's this.coprocessorHost variable is updated with a new RegionCoprocessorHost object that uses a CompoundConfiguration object in its constructor (rather than a Configuration object).

@kgeisz
HBASE-30180: Can still add data to read-only region after flipping re…
f31676e 
…ad-only flag multiple times

Code generated with Cursor:
  - Unit test TestHRegion.testRegionOnConfigurationChangeLoadsReadOnlyCoprocessorsAfterRepeatedToggle()

Code generated with Claude Opus:
  - Unit test TestHRegion.testRegionCoprocessorHostUsesCompoundConfigurationAfterConfigChange()

Change-Id: I8c5edf6ae4775d36dbbe2b260f77e2168a2a92da
@kgeisz kgeisz force-pushed the HBASE-30180-can-add-data-to-read-only-table branch from 82a3576 to f31676e Compare June 4, 2026 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@wchevreuil wchevreuil wchevreuil approved these changes

@taklwu taklwu Awaiting requested review from taklwu

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kgeisz @taklwu @wchevreuil